Codecov Report

❌ Patch coverage is 71.73913% with 91 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.40%. Comparing base (f38ae20) to head (af09d77).
⚠️ Report is 2 commits behind head on master.
✅ All tests successful. No failed tests found.

@@            Coverage Diff             @@
##           master    #3199      +/-   ##
==========================================
+ Coverage   82.32%   82.40%   +0.08%     
==========================================
  Files         613      613              
  Lines       36879    36982     +103     
  Branches     6047     6031      -16     
==========================================
+ Hits        30359    30475     +116     
+ Misses       5628     5622       -6     
+ Partials      892      885       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Hello @Nateowami ,

Thank you for your work on this!

Here are some comments on the code.

I find positive names to be easier to understand than negative names, when using boolean logic. I suggest to consider renaming DocSubscription.isUnsubscribed$ to isSubscribed$.

Can you explain more about the use of DocSubscription.unsubscribe() instead of using a destroyRef argument to DocSubscription.constructor? It looks like DestroyRef is for some angular classes, and so if we want to use a DocSubscription in a different situation, we might not have a DestroyRef available, and so we would use DocSubscription.unsubscribe?

Rather than provide DocSubscription.unsubscribe for situations where a DestroyRef|Observable was not provided to DocSubscription.constructor, do you think we could always require clients to do one of

• Give a DestroyRef that they have.
• inject a DestroyRef, and pass it to DocSubscription.constructor, and later the DestroyRef gets destroyed when.. when the "corresponding injector is destroyed"; or
• Pass an Observable, and emit from the Observable when the client wants to unsubscribe.
  I'm thinking this would reduce the complexity and variation of the DocSubscription class. (DocSubscription.unsubscribe could be removed.)
  I might have to have a look at what that looks like to decide if I like it better or worse :).

It looks like if we removed DocSubscription.unsubscribe, and instead had clients pass an Observable, that might look something like

// New client field
private readonly bye$ = new Subject<void>();
...
// Pass in constructor
new DocSubscription('DraftGenerationStepsComponent', this.bye$.asObservable())
...
// New client method
wave(): void {
  this.bye$.next();
  this.bye$.complete();
}

I want to mention that we could further reduce the complexity of DocSubscription by changing the constructor destroyRef argument from DestroyRef | Observable<void> to just Observable<void>. That would give some simplification.
Client code might change from

new DocSubscription('DraftGenerationStepsComponent', this.destroyRef)

to something like

new DocSubscription('DraftGenerationStepsComponent', new Observable(subscriber => this.destroyRef.onDestroy(() => subscriber.next())))

That makes the client code more complex just to simplify DocSubscription.constructor by a couple lines, so I'm not too inclined toward it. But I wanted to mention that idea in case it inspires other ideas.

Sometimes when working in TypeScript it seems like it could be useful to have a standard disposable system. In C#, there is an IDisposable interface, and implementing classes have a dispose() method that is called when the class is going away. In this dispose method you can let go of resources that you were holding onto.
If we had any desire to apply a generic disposal system across the frontend TypeScript application, this might be a good place to implement and do it.
AI suggested some disposal patterns that could be applied, which are

• Implement our own Disposable interface with a dispose method like C#.
• Using RxJS Subscription, where we add in each held resource and then in ngOnDestroy or another "dispose" method we let go of the resources.
• Using Angular DestroyRef. I wonder if we can use this with arbitrary classes, not just with Components and Directives.
• And of interest, in the future, using might come to TypeScript.

In C#, the IDisposal.dispose method is automatically called if you are using an object and it goes out of scope. (using someResource = foo()) Unfortunately, only the Angular DestroyRef and in-the-future using patterns above would be as automated (at least where we could not rely on ngOnDestroy). Hmm.
Well, do you have any comments on generic disposal ability in the frontend application?

@Nateowami Okay, we have a clean run of unit tests and e2e tests.

I am doing some more review of the PR before marking as 'Ready for review'.